-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix tap migration renames with API #17555
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense so far, thanks @apainintheneck!
I've been thinking about this more and I just wanted to get some opinions on the order of execution for the formula and cask loaders. This PR changes things slightly for cask and formula short names. The question is how to handle ambiguous cask/formula short names. Before
After
Does this sound reasonable? Would it be better to try to match the old behavior a bit more? That would probably involve modifying the name loader to call the API loader. |
Sounds reasonable, yes, thanks @apainintheneck ❤️ If it helps: in the medium-term I think we should avoid having homebrew/core and homebrew/cask names ever overlapping at all. Given they are both "official" and "auto-tapped": it's unnecessarily confusing for our names to overlap. Many users don't know (or care) about the differences between formulae and casks. That said: formulae are more widely used and should be the "default". |
We don't load casks and formulae that used to exist in an external tap but have been moved and renamed to an API tap when using the API. Loading works correctly when the core formula or cask tap is tapped locally though. This PR adds some logic to map tap migration renames to the API and check that when loading formulae and casks.
Check for the following: - Tap migration rename to core tap can be loaded by short name - Tap migration rename to core tap can be loaded by long name - Tap migration renam that clashes with existing core tap short name is ignored in favor of loading the cask/formula from the core tap
This changes slightly how we load packaged migrated to core taps when they are loaded using the API. Now it should show the warning that the package has been migrated and renamed.
fcaeae9
to
0c81a31
Compare
The loading logic should be correct now. I tweaked things so that the warning message that the package has been renamed shows up correctly now. I still need to do some manual testing to make sure that the |
!Homebrew::API::Formula.all_renames.key?(name) | ||
return | ||
|
||
ref = if (name = parse_name(ref)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on naming this something like:
ref = if (name = parse_name(ref)) | |
ref = if (name = parse_homebrew_core_name(ref)) |
Or something like that? That way it's clear that the "parsing" that's happening is only within the homebrew/core tap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe parse_core_name
here and parse_core_token
in Cask::CaskLoader
?
Closing in favor of #17599 |
fix-tap-migration-renames-with-api |
1 similar comment
fix-tap-migration-renames-with-api |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Fixes #17234
We don't load casks and formulae that used to exist in an external tap but have been moved and renamed to an API tap when using the API. Loading works correctly when the core formula or cask tap is tapped locally though.
This PR adds some logic to map tap migration renames to the API and check that when loading formulae and casks.
Todo: